Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update asn1 files #17

Merged
merged 14 commits into from
Dec 30, 2024
Merged

Update asn1 files #17

merged 14 commits into from
Dec 30, 2024

Conversation

xDimon
Copy link
Member

@xDimon xDimon commented Dec 13, 2024

Update code of parsing asn1 files in according with changes of jam-test-vectors.
Fix safrol test.

@xDimon xDimon requested review from kamilsa and turuslan December 13, 2024 14:02
src/TODO_qtils/bytes_std_hash.hpp Outdated Show resolved Hide resolved
src/jam/empty.hpp Outdated Show resolved Hide resolved

namespace jam {

template <typename T, typename = std::enable_if<std::is_scalar_v<T>>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use Foo = Tagged<T> instead of struct Foo { T v }?

Why inherit Tagged<T> : T {} instead of Tagged<T> { T v }?

Copy link
Member Author

@xDimon xDimon Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use Foo = Tagged instead of struct Foo { T v }?

Composition hides public members, and developers have to have access to them over composed member. It makes code a bit noisy and non clear. Tagged types more transparent.

Why inherit Tagged : T {} instead of Tagged { T v }?

Same explanation. But additionally for POD type is used zero-cost Wrapper.

/// Special zero-size-type for some things
/// (e.g., dummy types of variant, unsupported or experimental).
template <size_t N>
using Unused = Tagged<Empty, NumTag<N>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate struct, not related to Empty (encode/decode)

Suggested change
using Unused = Tagged<Empty, NumTag<N>>;
struct Unused {};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. std::variant can not contain two same type. Tagged type resolves this disadvantage.

#include <test-vectors/vectors.hpp>

namespace jam::test_vectors_disputes {
namespace jam::test_vectors::disputes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace test_vectors_disputes was intentional, to avoid
"no type/function foo in namespace disputes, did you mean jam::disputes::foo or jam::test_vectors::disputes::foo" error

.tau = tau_tick,
.eta = {eta_tick_0, eta_tick_1, eta_tick_2, eta_tick_3},
.lambda = lambda_tick,
.kappa = kappa_tick,
.gamma_k = gamma_tick_k,
// TODO(turuslan): #3, wait for test vectors
.iota = iota,
.gamma_a = gamma_tick_a,
.gamma_a = {gamma_tick_a.begin(), gamma_tick_a.end()},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.gamma_a = {gamma_tick_a.begin(), gamma_tick_a.end()},
.gamma_a = asVec(gamma_tick_a),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but I think in my implementation vector emplaced by iterators, but asVec creates temporary vector and moves it.

test-vectors/safrole/safrole.hpp Outdated Show resolved Hide resolved
test-vectors/safrole/safrole.hpp Outdated Show resolved Hide resolved
test-vectors/safrole/safrole.hpp Outdated Show resolved Hide resolved
test-vectors/disputes/disputes.hpp Show resolved Hide resolved
@xDimon xDimon merged commit 00211c1 into master Dec 30, 2024
2 checks passed
@xDimon xDimon deleted the update/asn1_files branch December 30, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants